Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove banner code and instead use should_show_banner #1027

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

lgoettgens
Copy link
Member

The current implementation does not work with julia 1.11 anymore. This has been fixed in the AA copy of this code in Nemocas/AbstractAlgebra.jl#1758.
All other of our packages have already switched from bundling their own version of this to calling the AA function. This PR does the same for GAP.jl.
I know that this introduces AA as a new dependency for GAP.jl (and Polymake.jl), please redirect comments concerning that to Nemocas/AbstractAlgebra.jl#1698.

This PR, if merged, should be backported to the 0.10.x branch that is used with the current Oscar release 1.1.1, so that users of that won't see too many banners once julia 1.11 is released.

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.71%. Comparing base (a8674da) to head (df44dea).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
- Coverage   76.73%   76.71%   -0.03%     
==========================================
  Files          51       51              
  Lines        4182     4178       -4     
==========================================
- Hits         3209     3205       -4     
  Misses        973      973              
Files with missing lines Coverage Δ
src/GAP.jl 85.58% <100.00%> (-0.51%) ⬇️

@fingolfin
Copy link
Member

Much more work is needed to support newer Julia versions with the old GAP.jl (and Polymake, Singular): each time a new Julia comes out, we must then update libjulia_jll and also the various JLLs for GAP, Polymake, Singular that link to it... And of course also the relevant libcxxwrap_julia_jll that they use. And that would mean updating these in the old versions that were in use by the OSCAR.jl 1.1.1 dependencies, even if in the meantime we have moved on to newer versions of CxxWrap/Singular/Polymake/GAP/FLINT/Normaliz/... .

Doing that is of course in principle possible, but would require a lot of carefully coordinated work and effort on our side, and possible some "infrastructure" that does not yet exist (e.g. right now our recipes in Yggdrasil are not really prepared to allow making rebuild-releases of old versions of a JLL).

I am not saying this to dismiss your point out of hand, though. I think we need to discuss this and decide whether we can afford to / want to / need to support OSCAR 1.0 / 1.1 / 1.x / ... on new Julia releases; or whether we keep doing what do now, which basically is: when there is a new Julia release, you most likely need to update to the latest stable OSCAR release. If you can't for some reason, you need to stick with the older Julia release.

While the current plan is of course not optimal for endusers, I have doubts that we have the resources to do anything else.

And given that juliaup makes it very easy to install older Julia versions in parallel to the most recent one, I am tempted to say we keep doing what we've done so far. But even if we agree to that, it should be a conscious explicit decision (and ideally documented somewhere). Same if we decide on something else.

CC @thofma @fieker @simonbrandhorst @micjoswig

@fingolfin
Copy link
Member

(BTW whether or not to merge this PR is independent of the larger discussion. I will evaluate that once I have fixed the GAP GC issues, which is a much higher priority to me right now, and unfortunately further delayed due to my dental issues)

@lgoettgens
Copy link
Member Author

I am not saying this to dismiss your point out of hand, though. I think we need to discuss this and decide whether we can afford to / want to / need to support OSCAR 1.0 / 1.1 / 1.x / ... on new Julia releases; or whether we keep doing what do now, which basically is: when there is a new Julia release, you most likely need to update to the latest stable OSCAR release.

I am well aware of the current state in the last sentence of the above quote block. However, OSCAR 1.1.1 is our current latest stable release, so for the upcoming julia 1.11 release to get supported I only see two possibilities:

  1. Backport changes like this to a branch that gets used by Oscar 1.1.1.
  2. Release some Oscar 1.2.x that uses a newer version of our deps. For most of them that wouldn't be a problem (up to deciding and coordinating when and how to release that), but in particular for GAP.jl there still is Update to GAP.jl 0.11.3 resp. GAP 4.13.1 Oscar.jl#3688 waiting to be merged.

@benlorenz
Copy link
Member

I did not look into why but at least right now the banner hiding for Oscar 1.1.1 does seem to work on julia-1.11.0-rc2. I only get the Oscar banner and no GAP banner.
The same even on julia nightly.

isquiet = Bool(Base.JLOptions().quiet)

show_banner = !isquiet && isinteractive_manual && isinteractive() &&
!any(x->x.name in ["Oscar"], keys(Base.package_locks)) &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!any(x->x.name in ["Oscar"], keys(Base.package_locks))

This part is different from the banner hiding code in AA. So for Oscar, the current version works aswell. But not for other packages that have GAP as a dependency.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not thrilled about the AA dependency, but it is what we agreed on so far, and Polymake.jl now also has it for the same reason, so let's do it.

@fingolfin fingolfin merged commit d53926d into oscar-system:master Sep 12, 2024
20 of 21 checks passed
@fingolfin
Copy link
Member

Thanks @lgoettgens

@lgoettgens lgoettgens deleted the lg/banner branch September 12, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants